[rig-sampler] fix: recurse into NullableSchema.inner in assertValidSchema - #33
Merged
Merged
Conversation
assertValidSchema validated array items, record values, and object
property schemas but skipped the inner schema of NullableSchema. A
misconfigured nested schema like s.nullable(s.object({ x: 'bad' as any }))
would silently escape the early construction-time check and only surface
a confusing error at invocation time or, worse, produce a repair loop.
This adds a nullable branch that recurses into schema.inner before the
properties branch, making schema validation consistent for all composite
schema types.
Discovered while running sample 13-test-plan.ts: the sample uses
s.nullable indirectly through s.object with enum fields, and analysing
assertValidSchema revealed the gap.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Contributor
Author
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed: The bug is fixed at the validation layer, not papered over at the call site. All composite schema types (array, record, object, nullable) now recurse uniformly.
- Regression test present: The new test case directly exercises the previously-uncovered path and asserts the correct error-path string (
output.score), making future regressions immediately visible.
Positive Highlights
- ✅ Placement of the nullable branch before the
propertiesbranch is correct — avoids a potential false-fall-through if inner is an ObjectSchema. - ✅ The
pathforwarding (no extra segment for nullable) is consistent with the semantics: nullable is a wrapper, not a path step. - ✅ All 135 existing tests continue to pass.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 19 AIC · ⌖ 6.65 AIC · ⊞ 6.3K
Comment /matt to run again
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sample run:
13-test-plan.tsRun outcome: The stub runner completed in 12 ms (1 turn, no repair). The agent output schema uses
s.enumands.array(s.object(...))— both well-structured, no repair loops needed.Analysis: While reading the harness code after the run I found a gap in
assertValidSchema. The function validates composite schemas by recursing into:ArraySchema.itemsRecordSchema.additionalPropertiesObjectSchema.properties...but it did not recurse into
NullableSchema.inner. That means a misconfigured schema like:silently passed the construction-time schema check. The invalid nested schema would only surface as a confusing error at invocation time — or, worse, generate a repair loop if the malformed schema was serialised into a prompt and the model tried (and failed) to match it.
Change
Added a
nullablebranch inassertValidSchemathat recurses intoschema.innerbefore thepropertiesbranch, making schema validation consistent for all composite schema types:A new test case in
src/rig.test.tsverifies the path reports the correct nested key in the error message (output.score).All 135 existing tests continue to pass.